Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the rust-version field in Cargo.toml to set minimum Rust version #506

Closed
wants to merge 1 commit into from

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented Aug 22, 2023

This encodes the minimum version so that the tooling will verify the user has a new enough Rust.

Set based on the needs from #498.

Fixes #394. Supersedes #499.

Cargo.toml Outdated Show resolved Hide resolved
@lschuermann
Copy link
Member

How does this relate to rust-toolchain? Would we eventually want to switch away from this entirely in favor of a MSRV?

@lschuermann lschuermann dismissed their stale review August 22, 2023 20:17

Changes addressed

@lschuermann
Copy link
Member

Seems like this needs #481.

@bradjc
Copy link
Contributor Author

bradjc commented Aug 22, 2023

Ok yeah this applies on top of https://rust-lang.github.io/rustup/overrides.html

This is confusing, because either rust-toolchain meets this requirement or nothing will build. But, since we also use stable for tests, this is useful for making sure the tests compile with a new enough rust.

@bradjc
Copy link
Contributor Author

bradjc commented Aug 22, 2023

Why don't we use stable by default and nightly only for miri?

@jrvanwhy
Copy link
Collaborator

Why don't we use stable by default and nightly only for miri?

I first introduced the check (#315) when only a subset of libtock-rs' crates built on stable, then later expanded the set (#390) to all crates. At the time, I had a few ideas for things other than Miri that required nightly, but I haven't had the time to implement them so at the moment we only need nightly for Miri.

I think it's fine to flip that around, and use the MSRV by default (e.g. via rust-toolchain and invoke our nightly version only for specific commands (at the moment, the Miri invocations).

@@ -8,6 +8,9 @@ license = "Apache-2.0 OR MIT"
name = "libtock"
repository = "https://www.github.com/tock/libtock-rs"
version = "0.1.0"
# Minimum required Rust compiler version for libtock-rs.
# Aug 2023: Set to support `Result::is_ok_and()`.
rust-version = "1.70"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be set workspace-wide and inherited.

... which requires a newer nightly toolchain, because the current toolchain is too old to support workspace inheritance.

Copy link
Member

@lschuermann lschuermann Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like someone should really be looking into updating the nightly toolchain, eh? 😆
I can attempt that tonight, if adding sptr as an external dependency is uncontroversial enough. All of the other proposals in #481 (especially mine!) seem ... less than suboptimal.

@@ -8,6 +8,9 @@ license = "Apache-2.0 OR MIT"
name = "libtock"
repository = "https://www.github.com/tock/libtock-rs"
version = "0.1.0"
# Minimum required Rust compiler version for libtock-rs.
# Aug 2023: Set to support `Result::is_ok_and()`.
rust-version = "1.70"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to test with this MSRV in Makefile rather than using +stable, otherwise this will bitrot.

@bradjc
Copy link
Contributor Author

bradjc commented Aug 24, 2023

Done in #513

@bradjc bradjc closed this Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"make test" fails
3 participants